-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a pull request template #17281
Add a pull request template #17281
Conversation
I'm sure we can fill this out with a lot more. For now, though, I just want a non-intrusive way to remind about autofix. Suggestions on other things to add would be appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably also add:
- Add tests.
- Use DCA
.github/pull_request_template.md
Outdated
### Pull Request checklist | ||
|
||
- [ ] Add a change if necessary. See [the documentation](https://github.com/github/codeql/blob/main/docs/change-notes.md). | ||
- [ ] If this PR makes any changes to `.ql`, `.qll`, or `.qhelp` files, make sure that autofixes generated based on these changes are valid. See [the documentation](https://github.com/github/codeql-team/blob/main/docs/best-practices/validating-autofix-for-query-changes.md) (internal access required). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [ ] If this PR makes any changes to `.ql`, `.qll`, or `.qhelp` files, make sure that autofixes generated based on these changes are valid. See [the documentation](https://github.com/github/codeql-team/blob/main/docs/best-practices/validating-autofix-for-query-changes.md) (internal access required). | |
- [ ] If this PR makes significant changes to `.ql`, `.qll`, or `.qhelp` files, make sure that autofixes generated based on these changes are valid. See [the documentation](https://github.com/github/codeql-team/blob/main/docs/best-practices/validating-autofix-for-query-changes.md) (internal access required). |
24415b6
to
c1c9ef2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but Esben should also have a look.
Adding codeql Engineering Managers as reviewers since we discussed this PR this morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo, otherwise happy with this as a starting point we can refine over time.
Co-authored-by: Aditya Sharad <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. But Esben should still have a look.
mentioning @esbena in case this was lost in the notifications maelstrom |
I'll keep this open until Tuesday so I can give others a chance to review. |
.github/pull_request_template.md
Outdated
|
||
#### All query authors | ||
|
||
- [ ] Add a change note if necessary. See [the documentation](https://github.com/github/codeql/blob/main/docs/change-notes.md) in this repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent style. I'm not going to discuss which of the two styles that is preferable.
- [ ] Add a change note if necessary. See [the documentation](https://github.com/github/codeql/blob/main/docs/change-notes.md) in this repository. | |
- [ ] Change notes are added if necessary. See [the documentation](https://github.com/github/codeql/blob/main/docs/change-notes.md) in this repository. |
This matches the declarative style of the other bullets, especially "QL tests are added..." one.
.github/pull_request_template.md
Outdated
- [ ] If this PR makes significant changes to `.ql`, `.qll`, or `.qhelp` files, make sure that autofixes generated based on these changes are valid. See [the documentation](https://github.com/github/codeql-team/blob/main/docs/best-practices/validating-autofix-for-query-changes.md) (internal access required). | ||
- [ ] Test your changes [at scale](https://github.com/github/codeql-dca/) (internal access required). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two bullets might need a reformulation to match the style above.
This PR adds a pull request template. It is sparse right now. The main goal is to remind query authors to check for autofix changes when they add new queries or update existing ones.